Do these above abstraction methods provide enough value to be worth their existence/DSL/maintenance? It seems like something similar to:

t.respond(options: {
  body?: HttpResponsePayload | ResponseError;
  statusCode: number;
  options?: HttpResponseOptions;
});

would be sufficient? Just thinking that most devs are familiar with HTTP, status codes, etc. and the DSL/abstraction may be more trouble than it's worth unless I'm missing context on why it's necessary.

The only advantage of a DSL that I can think of is much more control over the shape of responses e.g. a 404 can't return its own custom html markup and that a 204 can't return a body. But the value in doing so might be low.

But I agree with what you said an the drawbacks in the rfc:

KibanaResponseToolkit may not cover all use cases and requires an extension for specific use-cases.
KibanaResponseToolkit operates low-level Http primitives, such as Headers e.g., and it is not always handy to work with them directly.

The only advantage of a DSL that I can think of is much more control over the shape of responses e.g. a 404 can't return its own custom html markup and that a 204 can't return a body

right, it's easier for the platform to perform validation and we have some space to extend core functionality in future (for example, add a type definition that WWW-Authenticate header required when a user is rejected as unauthorized).

I love the idea of a simple DSL for validation & consistency; for me the only thing that's missing is a clear understanding of how the DSL translates to an actual http response payload. (But that might just be me)

IMO that shouldn't prevent us from going this direction -- but to @jasonrhodes' point, many devs are already going to be thinking this way, so making it clear exactly what these methods are doing for them will be important.

for me the only thing that's missing is a clear understanding of how the DSL translates to an actual http response payload. (But that might just be me)

For success responses, we pass payload to hapi server as is. As I understand, nothing has changed for plugin authors here.
Although, it defines different logic for the error responses. Hapi doesn't allow to respond with Error object, but our DSL does. For the plugin authors it looks like they throw Boom error in hapi request handler.

The only advantage of a DSL that I can think of is much more control over the shape of responses e.g. a 404 can't return its own custom html markup and that a 204 can't return a body. But the value in doing so might be low.

But I agree with what you said an the drawbacks in the rfc:

KibanaResponseToolkit may not cover all use cases and requires an extension for specific use-cases.
KibanaResponseToolkit operates low-level Http primitives, such as Headers e.g., and it is not always handy to work with them directly.

If a handler throws a KibanaResponse, the server should still use this thrown response to populate the actual HTTP response. Any other types of objects thrown should result in a 500.

It's not clear whether or not this is the case in this design.

A handler function should:

• do not throw an error.
• always return a KibanaResponse.

Interesting, so with this approach you are encouraged to construct the response in the handler code itself, rather than allow business logic to construct HTTP responses.

This encourages less coupling of our logic to the HTTP framework, which is a big plus.

// Without allowing exceptions (this RFC)
const async myHandler(context, req, t) {
  try {
    const obj = await retrieveObj(context.savedObjectClient, req.params.id);
    const res = processObj(obj);
    return t.ok(res);
  } catch (e) {
    // handler constructs the error response
    return t.notFound();
  }
}

// Allowing exceptions (not this RFC - more like Hapi)
const async myHandler(context, req, t) {
  // business logic construct and throw a 404 on it's own
  const obj = await retrieveObj(context.savedObjectClient, req.params.id, t);
  const res = processObj(obj);
  return t.ok(res);
}

• Throwing and re-throwing errors inside the code are not fun and hard to follow.
• If we mix all exceptions it's hard to type properly and distinguish between business errors & exception raised due to a bug.
• Core doesn't dictate how to handle errors. If a plugin wants to use Boom - ok, but it's not a part of the core contract anymore. If

I'd write your example as:

const async myHandler(context, req, t) {
    const obj = await retrieveObj(context.savedObjectClient, req.params.id);
    if (!obj) return t.notFound();
    const res = processObj(obj);
    return t.ok(res);
}

On the other hand, I understand that people are used to the Hapi feature to response with thrown Error and I'm open to introducing this functionality if needed as long as we allow throwing only KibanaResponseError object instead of Boom error.

I like that exceptions are used for exceptional or unexpected behaviour so I prefer the API of the RFC above the way Hapi does it 👍

Throwing and re-throwing errors inside the code are not fun and hard to follow.

++ I like how this removes a small element of "magic" and forces you to explicitly respond with errors in the handler itself. To me the approach in this RFC feels simpler, even if it isn't the Hapi way of doing things.

++, same here, I'd expect "HTTP related logic" (e.g. errors status codes or HTTP headers) to live inside HTTP handlers only where feasible.

Sounds like we're all on the same page here 🎉 . I just wanted to be clear on how this would work in the NP.

This may be out of the scope of this RFC.

In previous discussions, we've talked about having more explicit methods for registering routes. For example, we've talked about having a registerPublicApi and a registerInternalApi methods to distinguish between internal and public APIs.

Do we intend these functions to live on the HttpSetup contract and accept a Router or for them to live on the Router object itself?

In previous discussions, we've talked about having more explicit methods for registering routes. For example, we've talked about having a registerPublicApi and a registerInternalApi methods to distinguish between internal and public APIs.

Where can I find this discussion? I heard about something similar only once #33775 (comment)

I want to structure for myself what considered as public API and internal API? what is system api in this picture?

And yeah, we can create a separate issue to discuss

We discussed this offline at GAH Orlando as part of the discussion to provide OpenAPI specs for public APIs.

In my view:

• Public APIs are stable across minor releases, provide an OpenAPI spec, are documented publicly, and are exposed to other plugins via a JS client.
• Internal APIs are specific to a plugin and do not provide any stability guarantees.

It's not immediately obvious to me exactly what a "system API" is right now, but it seems like the same concept as an Internal API. I just think it should be more explicit (via the HTTP route instead of a header).

I'll move this to a separate discussion here: #40623

I know Error is an interface, but it's also a class which makes creating instances easy. Maybe there should be a convenience method for constructing instances of ResponseError.

you mean provide a class or a factory as well?

interface ResponseErrorType extends Error {
  meta?: {
    data?: JSONValue;
    errorCode?: string; // error code to simplify search, translations in i18n, etc.
    docLink?: string; // link to the docs
  };
}

class ResponseError extends Error implements ResponseErrorType {
  constructor(error: Error | string, public readonly meta: ResponseErrorType['meta']) {
    super(typeof error === 'string' ? error : error.message);
    Object.setPrototypeOf(this, ResponseError.prototype);
  }
}

// or a factory
const createError = (error: Error | string, meta?: ResponseErrorType['meta']) => 
  new ResponseError(error, meta)

Factory makes sense to me, seems more ergonomic / similar to the response factory methods.

You are missing a } here.

Throwing and re-throwing errors inside the code are not fun and hard to follow.

++ I like how this removes a small element of "magic" and forces you to explicitly respond with errors in the handler itself. To me the approach in this RFC feels simpler, even if it isn't the Hapi way of doing things.

I assume a more detailed design of the response payloads for various types of responses is outside the scope of this RFC?

It would be nice to provide some consistency in the payload, for example requiring a message in the payload for a 400 error. The response toolkit DSL would enable us to do this; I'm mostly wondering where/when this discussion would take place. (Or more broadly, how much of this we'd actually plan to enforce in core).

It would be nice to provide some consistency in the payload, for example requiring a message in the payload for a 400 error. The response toolkit DSL would enable us to do this;

Yeah, our own response toolkit is great, we can tighten signatures as much as we want. Feels like every such case (or group of related cases) would deserve a dedicated PR (not RFC, but at least PR). Another thing to keep in mind is that browsers are full of various quirks and legacy workarounds, so even if we tailor response helpers for the most common use cases we should leave an escape hatch to deal with all that Web weirdness in case it's needed (and I see we have it as custom response toolkit helper, that's great).

I'd start with more generic definitions outlined in this PR and then tighten them case by case in separate PRs

I love the idea of a simple DSL for validation & consistency; for me the only thing that's missing is a clear understanding of how the DSL translates to an actual http response payload. (But that might just be me)

IMO that shouldn't prevent us from going this direction -- but to @jasonrhodes' point, many devs are already going to be thinking this way, so making it clear exactly what these methods are doing for them will be important.

optional nit: maybe more specific link to the context description section instead of a link tfull RFC? https://github.com/elastic/kibana/blob/master/rfcs/text/0003_handler_interface.md#handler-context

note: known values - not really related to the RFC, but wondering if relying on IncomingHttpHeaders gives us much benefits. It can definitely makes our lives a bit harder though (unless we commit to patch it on the Kibana level or contribute to DT typings), e.g. with the current Node typings WWW-Authenticate supports only string while in fact it should support string | string[]. Some browsers (e.g. Firefox) better deal with separate headers, and have quite a few bugs when single WWW-Authenticate header includes multiple comma separated entries even though it's supported by the spec.

yeah, I noticed it. we can use only names of known headers to improve DX with autocomplete in IDE. on the whole this is optional step and may not implement it.

++, same here, I'd expect "HTTP related logic" (e.g. errors status codes or HTTP headers) to live inside HTTP handlers only where feasible.

It would be nice to provide some consistency in the payload, for example requiring a message in the payload for a 400 error. The response toolkit DSL would enable us to do this;

Yeah, our own response toolkit is great, we can tighten signatures as much as we want. Feels like every such case (or group of related cases) would deserve a dedicated PR (not RFC, but at least PR). Another thing to keep in mind is that browsers are full of various quirks and legacy workarounds, so even if we tailor response helpers for the most common use cases we should leave an escape hatch to deal with all that Web weirdness in case it's needed (and I see we have it as custom response toolkit helper, that's great).

question: is there any valid use case in KIbana that we treat it as a drawback?

It's not a drawback per se.
We have custom helper as escape-hatch, but if one doesn't cover a very weird use case it will take some time to extend the core.